fix: eliminate keyboard shortcuts UI flash#1021
fix: eliminate keyboard shortcuts UI flash#1021francoischalifour merged 5 commits intoalgolia:nextfrom
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 55b9e7a:
|
shortcuts
left a comment
shortcuts
left a comment
There was a problem hiding this comment.
Thanks for your PR, It indeed looks better! Small comment, I think using min-width might be safer
Co-authored-by: Clément Vannicatte <20689156+shortcuts@users.noreply.github.com>
| margin: 0 0 0 16px; | ||
| padding: 0 8px; | ||
| user-select: none; | ||
| min-width: 155px; |
There was a problem hiding this comment.
how do we know the value is 155px?
There was a problem hiding this comment.
I just inspected the element for the value:
Not a exact one, but it works in the webpack site case.
There was a problem hiding this comment.
The computed size of the component loaded is 155.359px, I assume that's where @chenxsan also found it.
There was a problem hiding this comment.
What if a different font, placeholder or zoom level is used? I guess it's still a safe default, but I think in some cases it will still flash
There was a problem hiding this comment.
I think users could always customize it to their own use cases. We're just providing a default one which should hopefully eliminate the flash for most of the cases. For instance, the value would work for https://docusaurus.io/ as well.
There was a problem hiding this comment.
What if a different font, placeholder or zoom level is used?
Most of these won't have effect unless the user is already overriding the provided styles. I think this could be a safe default and it is requested quite frequently.
wdyt? @Haroenv
There was a problem hiding this comment.
It's safer with the min- change IMO.
But I agree with Haroen. I feel like setting the constraint on .DocSearch-Button-Keys is better because that's ultimately what we want as constant.
There was a problem hiding this comment.
@francoischalifour I don't think it would work on .DocSearch-Button-Keys considering it's controlled by a state value
There was a problem hiding this comment.
Ah right, we can still move that check one line below to always render that element though?
There was a problem hiding this comment.
Thanks for bringing up this as I found a problem with my solution.
Depending on the key state, we have two cases :
- a search box with the keys
- a search box without the keys
When there's no keys, the current solution would be deficient, as it results in a wide search box:
I'll see what I can do with it.
Haroenv
left a comment
Haroenv
left a comment
There was a problem hiding this comment.
overall we rely on px too much, but this is a sensible default
chenxsan
changed the title
|
I've refactor the code as the former proposal won't cover all use cases, see #1021 (comment) for detail. Screen.Recording.2021-07-21.at.8.07.24.PM.movI've also set up a demo here https://stackblitz.com/edit/nextjs-vxz1zj?file=pages%2Findex.js so you can check. Let me know what you think about the new code. |
|
@chenxsan Unfortunately we cannot change the |
|
@francoischalifour You are right! I didn't notice it's public API. I've updated the code. |
francoischalifour
left a comment
francoischalifour
left a comment
There was a problem hiding this comment.
Much simpler now, thank you @chenxsan!
francoischalifour
changed the title
|
@francoischalifour @shortcuts Unfortunately it doesn't fix the flashing problem. I've filed a pull request on webpack documentation here webpack/webpack.js.org#5224, and here's the preview url https://webpack-js-org-git-fork-chenxsan-upgrade-do-a4186d-webpack-docs.vercel.app/, you can compare it with https://webpack.js.org/ which has the width set for the search box. |
4 participants
There's a flash of unstyled content regarding the search box:
Screen.Recording.2021-07-16.at.7.28.47.PM.mov
You can check it on https://docsearch.algolia.com/
Setting a width would eliminate the problem, you can check it on https://webpack.js.org/ for how it looks.